-
Couldn't load subscription status.
- Fork 15k
[Clang] Add __builtin_bswapg #162433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[Clang] Add __builtin_bswapg #162433
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: clf (clingfei) ChangesAdd a new builtin function __builtin_bswapg. It works on any integral types that has a multiple of 16 bits as well as a single byte, as described in #160266. Full diff: https://github.com/llvm/llvm-project/pull/162433.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index 468121f7d20ab..e65ed2f20be97 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -755,6 +755,12 @@ def BSwap : Builtin, Template<["unsigned short", "uint32_t", "uint64_t"],
let Prototype = "T(T)";
}
+def BSwapg : Builtin {
+ let Spellings = ["__builtin_bswapg"];
+ let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
+ let Prototype = "int(...)";
+}
+
def Bitreverse : BitInt8_16_32_64BuiltinsTemplate, Builtin {
let Spellings = ["__builtin_bitreverse"];
let Attributes = [NoThrow, Const, Constexpr];
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 618e1636e9e53..058905e7fd3c0 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -13982,6 +13982,17 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
return Success(Val.reverseBits(), E);
}
+ case Builtin::BI__builtin_bswapg: {
+ APSInt Val;
+ if (!EvaluateInteger(E->getArg(0), Val, Info))
+ return false;
+ if (Val.getBitWidth() == 8) {
+ bool ret = Success(Val, E);
+ return ret;
+ }
+
+ return Success(Val.byteSwap(), E);
+ }
case Builtin::BI__builtin_bswap16:
case Builtin::BI__builtin_bswap32:
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 063db05665af1..362b53676feaa 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2200,6 +2200,30 @@ static bool BuiltinCpu(Sema &S, const TargetInfo &TI, CallExpr *TheCall,
return false;
}
+/// Checks that __builtin_bswapg was called with a single argument, which is an
+/// unsigned integer, and overrides the return value type to the integer type.
+static bool BuiltinBswapg(Sema &S, CallExpr *TheCall) {
+ if (S.checkArgCount(TheCall, 1))
+ return true;
+ ExprResult ArgRes = S.DefaultLvalueConversion(TheCall->getArg(0));
+ if (ArgRes.isInvalid())
+ return true;
+
+ Expr *Arg = ArgRes.get();
+ TheCall->setArg(0, Arg);
+
+ QualType ArgTy = Arg->getType();
+
+ if (!ArgTy->isIntegerType()) {
+ S.Diag(Arg->getBeginLoc(), diag::err_builtin_invalid_arg_type)
+ << 1 << /* scalar */ 1 << /* unsigned integer ty */ 1 << /* no fp */ 0
+ << ArgTy;
+ return true;
+ }
+ TheCall->setType(ArgTy);
+ return false;
+}
+
/// Checks that __builtin_popcountg was called with a single argument, which is
/// an unsigned integer.
static bool BuiltinPopcountg(Sema &S, CallExpr *TheCall) {
@@ -3448,6 +3472,10 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
}
break;
}
+ case Builtin::BI__builtin_bswapg:
+ if (BuiltinBswapg(*this, TheCall))
+ return ExprError();
+ break;
case Builtin::BI__builtin_popcountg:
if (BuiltinPopcountg(*this, TheCall))
return ExprError();
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd think this is something that @AaronBallman or similar should approve (spelling at least).
There should be actual documentation about this in LanguageExtensions.rst (it seems like like the existing ones don't have documentation, but maybe they should be placed into a group just documenting the family of builtins), and a note in ReleaseNotes.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to allow bswap of _BitInt values?
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,c -- clang/test/SemaCXX/builtin-bswapg.cpp clang/lib/AST/ByteCode/InterpBuiltin.cpp clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGBuiltin.cpp clang/lib/Sema/SemaChecking.cpp clang/test/AST/ByteCode/builtin-functions.cpp clang/test/CodeGen/builtins.c clang/test/CodeGenCXX/builtins.cpp clang/test/Sema/constant-builtins-2.c clang/test/Sema/constant-builtins.c --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 38b9e0fed..428cf1be0 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2218,7 +2218,7 @@ static bool BuiltinBswapg(Sema &S, CallExpr *TheCall) {
if (!ArgTy->isIntegerType()) {
S.Diag(Arg->getBeginLoc(), diag::err_builtin_invalid_arg_type)
- << 1 << /*scalar=*/ 1 << /*unsigned integer=*/ 1 << /*floating point=*/ 0
+ << 1 << /*scalar=*/1 << /*unsigned integer=*/1 << /*floating point=*/0
<< ArgTy;
return true;
}
|
I believe I've got it back to the right set :D How did you update your branch? (My suspicion is that something about that caused GitHub to suggest every person involved in all the merged PRs) |
Thanks!
I performed a local rebase onto the main branch and pushed to my own branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be missing testing for _BitInt.
| case Builtin::BI__builtin_bswapg: { | ||
| Value *ArgValue = EmitScalarExpr(E->getArg(0)); | ||
| llvm::IntegerType *IntTy = cast<llvm::IntegerType>(ArgValue->getType()); | ||
| assert(IntTy && "LLVM's __builtin_bswapg only supports integer variants"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably have a getBitWidth() % 8 == 0 and getBitWidth() > 0, as a defense for future support of _BitInt if we don't simply include that now (which I think someone has already asked about?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @philnik777 described in #160266, bswapg should work on any integral types that have a multiple of 16 bits as well as a single byte. Thus, the condition should be (getBitWidth() % 16 == 0 and getBitWidth() > 0) or getBitWidth() == 8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think mod 16 is right either, swapping _BitInt(24) (etc) is in principle just as reasonable a swap, though it would be a novel choice. And at the same time as disallowing _BitInt(24), the mod 16 rule allows the equivalently odd _BitInt(48) to be swapped.
Would it be more reasonable to simply say it must be a power of 2 number of bytes (N=8*2^^N for some positive N)? @philnik777 does that sound reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds reasonable, but the llvm intrinsic has the constraint on 16 bits. I don't think we should block introducing the builtin on extending the llvm intrinsic.
It's probably just GH being weird then - it doesn't do well with rebasing. I haven't seen this particular result before, but that doesn't mean it won't. I believe GH prefers merging main into your branch (though I agree that rebasing is much more sensible). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a release note.
| int h7 = __builtin_bswapg((short)(0x1234)) == (short)(0x3412) ? 1 : f(); | ||
| int h8 = __builtin_bswapg(0x00001234) == 0x34120000 ? 1 : f(); | ||
| int h9 = __builtin_bswapg(0x0000000000001234ULL) == 0x3412000000000000 ? 1 : f(); | ||
| float h10 = __builtin_bswapg(1.0f); // expected-error {{1st argument must be a scalar integer type (was 'float')}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about also testing: arrays, pointers, enums, class, nullptr_t.
What do we expect w/ charN_t, wchar_t, bool and int128_t types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing for arrays, pointers, enums, class, nullptr_t has been added.
What do we expect w/ charN_t, wchar_t, bool and int128_t types?
Currently __builtin_bswapg treats these types the same way as the corresponding byte integer types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the additional test, should we add a _BitInt test @AaronBallman
|
The test for _BitInt has been added. |
| case Builtin::BI__builtin_bswapg: { | ||
| Value *ArgValue = EmitScalarExpr(E->getArg(0)); | ||
| llvm::IntegerType *IntTy = cast<llvm::IntegerType>(ArgValue->getType()); | ||
| assert(IntTy && "LLVM's __builtin_bswapg only supports integer variants"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think mod 16 is right either, swapping _BitInt(24) (etc) is in principle just as reasonable a swap, though it would be a novel choice. And at the same time as disallowing _BitInt(24), the mod 16 rule allows the equivalently odd _BitInt(48) to be swapped.
Would it be more reasonable to simply say it must be a power of 2 number of bytes (N=8*2^^N for some positive N)? @philnik777 does that sound reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me sans the mod 16 bit rule vs 8*2^^N question
…wap to support bswap for values with more bits
Add a new builtin function __builtin_bswapg. It works on any integral types that has a multiple of 16 bits as well as a single byte.
Closes #160266